Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add function to check if cache contains a key #382

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Darkheir
Copy link

Add a new function allowing to check if the cache contains the given key

@Darkheir Darkheir force-pushed the feat/contains_check branch from eb3ed48 to 0086967 Compare October 28, 2023 09:26
@janisz
Copy link
Collaborator

janisz commented Oct 31, 2023

Do we need this func? We have Get

@Darkheir
Copy link
Author

The main difference is that Get increases the hit score of the key while Contains doesn't.

I also find it more readable to do if !cache.Contains("key") than if _, err := cache.Get("foo"); errors.Is(err, bigcache.ErrEntryNotFound)

cristaloleg
cristaloleg previously approved these changes Nov 1, 2023
Copy link
Collaborator

@cristaloleg cristaloleg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM

@codecov-commenter
Copy link

codecov-commenter commented Nov 1, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.31%. Comparing base (e5c39b2) to head (6a482f4).
Report is 1 commits behind head on main.

Current head 6a482f4 differs from pull request most recent head 29b0a38

Please upload reports for the commit 29b0a38 to get more accurate results.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #382      +/-   ##
==========================================
+ Coverage   89.08%   89.31%   +0.23%     
==========================================
  Files          15       15              
  Lines         797      805       +8     
==========================================
+ Hits          710      719       +9     
  Misses         73       73              
+ Partials       14       13       -1     
Files Coverage Δ
bigcache.go 93.18% <100.00%> (+0.81%) ⬆️
shard.go 92.22% <100.00%> (+0.20%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a2f05d7...29b0a38. Read the comment docs.

Copy link
Collaborator

@janisz janisz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since @cristaloleg is fine with extending the interface let's merge it. Just one nitpic

shard.go Outdated Show resolved Hide resolved
return false
}
entryKey := readKeyFromEntry(wrappedEntry)
s.lock.RUnlock()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cristaloleg do you think we should increment hit count here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deeply philosophical question. I'm fine to not increment it here. Leave it only for real value get.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants